fix: strip any sub-paths for registries in ocmconfig - #424
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe OCM workflow template now removes URL paths before port information when it extracts source and destination registry hostnames. ChangesOCM Cluster Workflow Template
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
fd04c59 to
886afc9
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/ocm/cluster-workflow-template.yaml`:
- Line 123: The hostname parsing in the workflow template is stripping off the
port as well as the repo path, which breaks host matching for registries like
dst.zot:443. Update the expression that builds hostname in the affected workflow
steps so it removes only the repository sub-path from
workflow.parameters.srcRemoteURL while preserving the full host:port identity;
use the existing hostname assignment logic in the cluster workflow template as
the place to make the change and keep the port intact everywhere this pattern
appears.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad8a25f1-81c5-4888-baba-7863357f1383
📒 Files selected for processing (1)
examples/ocm/cluster-workflow-template.yaml
| - identity: | ||
| type: OCIRegistry | ||
| hostname: $(echo '{{workflow.parameters.srcRemoteURL}}' | cut -d : -f 1) | ||
| hostname: $(echo '{{workflow.parameters.srcRemoteURL}}' | cut -d / -f 1 | cut -d : -f 1) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve host:port when stripping the repo sub-path.
cut -d : -f 1 changes the registry identity, not just the path. That will break credential matching for registries exposed on an explicit port (for example dst.zot:443, which is already used elsewhere). This change should strip only the /repo suffix and keep any port intact.
Suggested fix
- hostname: $(echo '{{workflow.parameters.srcRemoteURL}}' | cut -d / -f 1 | cut -d : -f 1)
+ hostname: $(echo '{{workflow.parameters.srcRemoteURL}}' | cut -d / -f 1)
...
- hostname: $(echo '{{workflow.parameters.dstRemoteURL}}' | cut -d / -f 1 | cut -d : -f 1)
+ hostname: $(echo '{{workflow.parameters.dstRemoteURL}}' | cut -d / -f 1)Also applies to: 208-208
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/ocm/cluster-workflow-template.yaml` at line 123, The hostname
parsing in the workflow template is stripping off the port as well as the repo
path, which breaks host matching for registries like dst.zot:443. Update the
expression that builds hostname in the affected workflow steps so it removes
only the repository sub-path from workflow.parameters.srcRemoteURL while
preserving the full host:port identity; use the existing hostname assignment
logic in the cluster workflow template as the place to make the change and keep
the port intact everywhere this pattern appears.
There was a problem hiding this comment.
@trevex do you remember why we split on :. Was it for repo:tag (does the srcRemoteURL have a tag?) or host:port?
There was a problem hiding this comment.
@olzemal — looking at how srcRemoteURL is used elsewhere in the template, it appears the split on : was for host:port, not repo:tag. The tag portion (:specVersion) is appended separately via the constructed component_url:
component_url={{workflow.parameters.srcRemoteURL}}/{{workflow.parameters.specRepo}}//{{workflow.parameters.specComponent}}:{{workflow.parameters.specVersion}}So srcRemoteURL itself is expected to be something like registry.example.com, registry.example.com:443, or (after this PR) registry.example.com/my-repo — no tag component.
That's exactly why my original comment flagged the concern: the cut -d : -f 1 in the original code, and retained in this PR after cut -d / -f 1, strips the port (e.g. dst.zot:443 → dst.zot). If OCM does need the full host:port for credential matching on non-standard ports, that would be a problem. If OCM only ever matches on the bare hostname, then stripping the port is intentional and correct — worth confirming against the OCM documentation.
There was a problem hiding this comment.
Honestly not sure...
It was most likely due to port but might be because of ocm CLI quirkiness, so if it still works with explicit port we should include the port.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
886afc9 to
6912647
Compare
What
Handling registries like
example.com/my-repo. OCM expects credentials to be configured for justexample.com. We did not strip any possible subpaths prior to this change.Checklist
Tests added/updatedSummary by CodeRabbit